Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix downcasting bug in local_[mul|div]_switch_sink rewrite #1059

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Oct 31, 2024

Closes #1037

It also refactors the two rewrites to avoid so much duplicated code

The fix revealed some subtle bugs in the C implementation of Scalar Ops with negative literal constants, that were also fixed.


📚 Documentation preview 📚: https://pytensor--1059.org.readthedocs.build/en/1059/

@ricardoV94 ricardoV94 added bug Something isn't working graph rewriting labels Oct 31, 2024
@ricardoV94 ricardoV94 changed the title Fix bug in zero mul/div sink rewrite Fix bug in local_[mul|div]_switch_sink rewrite Oct 31, 2024
@ricardoV94 ricardoV94 force-pushed the zero_sink_rewrite_bug branch from 956b73f to faa1abb Compare October 31, 2024 10:24
@@ -2115,7 +2117,6 @@ def test_local_mul_switch_sink(self):
f = self.function_remove_nan([x], pytensor.gradient.grad(y, x), self.mode)
assert f(5) == 1, f(5)

@pytest.mark.slow
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not particularly slow and not slower than the test above

@ricardoV94 ricardoV94 force-pushed the zero_sink_rewrite_bug branch from faa1abb to ecf9829 Compare October 31, 2024 10:26
@ricardoV94 ricardoV94 changed the title Fix bug in local_[mul|div]_switch_sink rewrite Fix downcasting bug in local_[mul|div]_switch_sink rewrite Oct 31, 2024
@ricardoV94 ricardoV94 force-pushed the zero_sink_rewrite_bug branch from ecf9829 to e664da8 Compare October 31, 2024 11:58
@ricardoV94 ricardoV94 force-pushed the zero_sink_rewrite_bug branch from 2dffc91 to 096f285 Compare October 31, 2024 12:28
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.95%. Comparing base (6132203) to head (096f285).
Report is 97 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1059      +/-   ##
==========================================
- Coverage   81.95%   81.95%   -0.01%     
==========================================
  Files         182      182              
  Lines       47872    47857      -15     
  Branches     8618     8619       +1     
==========================================
- Hits        39233    39219      -14     
  Misses       6472     6472              
+ Partials     2167     2166       -1     
Files with missing lines Coverage Δ
pytensor/scalar/basic.py 80.52% <100.00%> (ø)
pytensor/scalar/loop.py 88.88% <100.00%> (ø)
pytensor/tensor/rewriting/math.py 89.82% <100.00%> (-0.05%) ⬇️
pytensor/tensor/variable.py 87.85% <100.00%> (+0.02%) ⬆️

@ricardoV94 ricardoV94 merged commit 4f7d709 into pymc-devs:main Oct 31, 2024
61 checks passed
@ricardoV94 ricardoV94 deleted the zero_sink_rewrite_bug branch October 31, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C-backend graph rewriting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downcasting in local_[mul|div]_switch_sink rewrite
2 participants